-
Notifications
You must be signed in to change notification settings - Fork 35
cmake: Increase cmake policy version #209
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
Updated 6e29c02 -> 233b394 ( Updated 233b394 -> 31206fc ( Updated 31206fc -> e6fd980 ( Updated e6fd980 -> c33b812 ( Updated c33b812 -> 1c2958b ( |
With latest cmake policies, find_package(Threads REQUIRED) fails in freebsd and openbsd builds. The specific failures in these jobs happens due to the CMP0155 policy turning CMAKE_CXX_SCAN_FOR_MODULES on, which is turned off in the next commit. But there are other things that could cause the threads package not to be found, and there are platforms where it may not be required, so it is better to make it an optional instead of required dependency. Technically this change is not needed to make all CI jobs pass as long as the next commit is present. But since this find_package error obscures other errors that would be clearer, and since there are other conditions that could trigger it, it is worth fixing generally. For example, this same failure also seems to happen in the llvm job as well when CMP0137 is disabled or CMAKE_TRY_COMPILE_NO_PLATFORM_VARIABLES is set to true. Error looks like: + cmake /home/runner/work/libmultiprocess/libmultiprocess -G Ninja -- The CXX compiler identification is Clang 16.0.6 -- Detecting CXX compiler ABI info -- Detecting CXX compiler ABI info - done -- Check for working CXX compiler: /usr/bin/c++ - skipped -- Detecting CXX compile features -- Detecting CXX compile features - done -- Performing Test CMAKE_HAVE_LIBC_PTHREAD -- Performing Test CMAKE_HAVE_LIBC_PTHREAD - Failed -- Looking for pthread_create in pthreads -- Looking for pthread_create in pthreads - not found -- Looking for pthread_create in pthread -- Looking for pthread_create in pthread - not found -- Check if compiler accepts -pthread -- Check if compiler accepts -pthread - no CMake Error at /usr/local/share/cmake/Modules/FindPackageHandleStandardArgs.cmake:233 (message): Could NOT find Threads (missing: Threads_FOUND) Call Stack (most recent call first): /usr/local/share/cmake/Modules/FindPackageHandleStandardArgs.cmake:603 (_FPHSA_FAILURE_MESSAGE) /usr/local/share/cmake/Modules/FindThreads.cmake:226 (FIND_PACKAGE_HANDLE_STANDARD_ARGS) CMakeLists.txt:41 (find_package) and inside the CMakeConfigureLog.yaml file there are "/bin/sh: CMAKE_CXX_COMPILER_CLANG_SCAN_DEPS-NOTFOUND: not found" errors.
31206fc
to
e6fd980
Compare
… cmake policies With latest cmake policies, openbsd and freebsd CI jobs fail due to a lack of a clang-scan-deps tool. The tool could potentially be installed on these platforms but it is unclear how to do that and the project isn't using modules anyway, so just disable them here. Errors look like: + cmake --build . --parallel -t all tests mpexamples -- -k 0 [1/114] Scanning /home/runner/work/libmultiprocess/libmultiprocess/src/mp/util.cpp for CXX dependencies FAILED: CMakeFiles/mputil.dir/src/mp/util.cpp.o.ddi "CMAKE_CXX_COMPILER_CLANG_SCAN_DEPS-NOTFOUND" -format=p1689 -- /usr/bin/c++ -I/home/runner/work/libmultiprocess/libmultiprocess/include -I/home/runner/work/libmultiprocess/libmultiprocess/build-openbsd/include -isystem /usr/local/include -Werror -Wall -Wextra -Wpedantic -Wno-unused-parameter -std=gnu++20 -x c++ /home/runner/work/libmultiprocess/libmultiprocess/src/mp/util.cpp -c -o CMakeFiles/mputil.dir/src/mp/util.cpp.o -resource-dir "/usr/lib/clang/16" -MT CMakeFiles/mputil.dir/src/mp/util.cpp.o.ddi -MD -MF CMakeFiles/mputil.dir/src/mp/util.cpp.o.ddi.d > CMakeFiles/mputil.dir/src/mp/util.cpp.o.ddi.tmp && mv CMakeFiles/mputil.dir/src/mp/util.cpp.o.ddi.tmp CMakeFiles/mputil.dir/src/mp/util.cpp.o.ddi /bin/sh: CMAKE_CXX_COMPILER_CLANG_SCAN_DEPS-NOTFOUND: not found
Increase cmake policy version from 3.12 to 4.1 in standalone builds to stop using very old and deprecated CMake policies in standalone builds. Also stop overriding policy version if a parent project has already set one, so parent projects are able to control which policies are used by default based on their own practices and needs. In the Bitcoin Core subtree, this change causes the libmultiprocess policy version to increase from 3.12 to 3.22, which is the version Bitcoin Core sets. This commit only changes the policy version, it does not change the specified minimum version of cmake required to build the project, which is 3.12.
e6fd980
to
c33b812
Compare
c33b812
to
1c2958b
Compare
cmake --build . --parallel -t "${BUILD_TARGETS[@]}" -- "${BUILD_ARGS[@]+"${BUILD_ARGS[@]}"}" | ||
else | ||
# Older versions of cmake can only build one target at a time with --target, | ||
# and do not support -t shortcut | ||
for t in "${BUILD_TARGETS[@]}"; do | ||
cmake --build . --target "$t" -- "${BUILD_ARGS[@]+"${BUILD_ARGS[@]}"}" | ||
cmake --build . --parallel --target "$t" -- "${BUILD_ARGS[@]+"${BUILD_ARGS[@]}"}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the --parallel
option is used without an argument:
the native build tool's default number is used.
This means that the build will be efficiently parallelized with the "Ninja" generator, but not with the "Unix Makefiles" generator. However, not all scripts in ci/configs/
pass -G Ninja
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re: #209 (comment)
This means that the build will be efficiently parallelized with the "Ninja" generator, but not with the "Unix Makefiles" generator. However, not all scripts in
ci/configs/
pass-G Ninja
.
On my system I see it passing -j
to make
which seems to work well in practice because this is a small build. Would happily accept a PR or patch to do something different, but I think this change is an improvement.
# Could add --trace / --trace-expand here too but they are very verbose. | ||
cmake_args+=(--debug-find --debug-output --debug-trycompile --log-level=DEBUG) | ||
cmake "$src_dir" "${cmake_args[@]}" || : "cmake exited with $?" | ||
cat CMakeFiles/CMakeConfigureLog.yaml || true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This log file name has been used only since CMake 3.26.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re: #209 (comment)
This log file name has been used only since CMake 3.26.
Yes that's part of the reason for adding || true
here. I think this code could potentially print log files used by older versions of cmake too, and originally this change tried that, but since all CI jobs except one are using new enough versions of cmake it didn't seem worth complexity. Would happily accept a PR to patch to improve this though.
I am not happy with this. The title of the topic says "Increase cmake policy version", which I would expect to be a one line change. Instead, it has +95/-17 changes. The changes themselves are questionable:
Please don't proceed in this direction. Make sure the |
I'm struggling to create a MRE for the issue with the cmake_minimum_required(VERSION 3.12...4.1)
project(freebsd-threads CXX)
set(CMAKE_CXX_STANDARD 20)
set(CMAKE_CXX_STANDARD_REQUIRED YES)
find_package(Threads REQUIRED) What am I missing? |
re: #209 (comment)
Happy to change the title if you have a suggestion. This PR is only increasing the policy version and improving the CI, and I did not feel the CI changes were were mentioning since they are supporting changes, not the goal of the PR.
I think the language question should be raised new issue if you want to discuss it. (But just to provide a little context here if it can avoid an argument about the language of a ~70 line script, I just personally find bash a lot nicer and more useful than cmake and am much familiar with it. More generally, bash is commonly used in CI, there many more developers who know bash than cmake.) The script does increase from 37 to 67 lines here to do two things:
IMO both changes are worth making and do not have any real downsides.
Interesting point. It might be helpful if you could explain practical downsides of the change. Unless you also believe that if you have to explain why a change is wrong, then it must be right?
Can you explain this a little more as well? Is there a practical downside here or a different approach you would recommend? |
re: #209 (comment)
The error happens when you don't have If it helps there is a failing CI run with more complete output: https://github.com/ryanofsky/libmultiprocess/actions/runs/17622518177/job/50071319558 And the relevant cmake policy is https://cmake.org/cmake/help/latest/policy/CMP0155.html |
To be sure, I agree with you it would be better if Bitcoin Core followed cmake's recommended best practices and used new policies instead of setting an really old policy version. But bitcoin core isn't doing that, and other bitcoin core developers have given reasons why it shouldn't do that, and while I don't agree with these reasons, I think it is a reasonable judgement call, and they are just choosing a different tradeoffs than I would. This PR makes libmultiprocess compatible with either approach for choosing a policy version and stops using really old policies (older than the ones bitcoin core chooses) in any case. |
Here is a build log from GHA using FreeBSD:
|
Is that link right? It seems to be a 404 for me. I also don't think I understand what it indicates if find_package(Threads REQUIRED) works in another build. We already know it works in many builds. You can see some builds where it is failing |
Sorry. I've made that repo public. The link should work now.
I want to report the issue upstream and I need an MRE for it. |
Oh, thanks! It seems like your branch is https://github.com/hebasto/github-actions/compare/250915-freebsd-cmake-threads and I agree I don't see an obvious reason why that CI succeeds while my CI runs such as https://github.com/ryanofsky/libmultiprocess/actions/runs/17622518177/job/50071319558 with branch https://github.com/ryanofsky/libmultiprocess/commits/citest/policy at 3b8a0e6 failed. Some things I might suggest:
|
Also, I don't think there is necessarily there is necessarily an upstream bug here, since we are asking cmake to look for a package and it fails and reports the failure. It just doesn't seem to do a good job reporting the cause of the failure. |
Thank you for the suggestion. The culprit turned out to be the "Ninja" generator, which was quite surprising to me. |
Nice! And that is really surprising. I would expect the generator not to matter much during configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review! Note: first two commits are now part of #212
# Could add --trace / --trace-expand here too but they are very verbose. | ||
cmake_args+=(--debug-find --debug-output --debug-trycompile --log-level=DEBUG) | ||
cmake "$src_dir" "${cmake_args[@]}" || : "cmake exited with $?" | ||
cat CMakeFiles/CMakeConfigureLog.yaml || true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re: #209 (comment)
This log file name has been used only since CMake 3.26.
Yes that's part of the reason for adding || true
here. I think this code could potentially print log files used by older versions of cmake too, and originally this change tried that, but since all CI jobs except one are using new enough versions of cmake it didn't seem worth complexity. Would happily accept a PR to patch to improve this though.
cmake --build . --parallel -t "${BUILD_TARGETS[@]}" -- "${BUILD_ARGS[@]+"${BUILD_ARGS[@]}"}" | ||
else | ||
# Older versions of cmake can only build one target at a time with --target, | ||
# and do not support -t shortcut | ||
for t in "${BUILD_TARGETS[@]}"; do | ||
cmake --build . --target "$t" -- "${BUILD_ARGS[@]+"${BUILD_ARGS[@]}"}" | ||
cmake --build . --parallel --target "$t" -- "${BUILD_ARGS[@]+"${BUILD_ARGS[@]}"}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re: #209 (comment)
This means that the build will be efficiently parallelized with the "Ninja" generator, but not with the "Unix Makefiles" generator. However, not all scripts in
ci/configs/
pass-G Ninja
.
On my system I see it passing -j
to make
which seems to work well in practice because this is a small build. Would happily accept a PR or patch to do something different, but I think this change is an improvement.
Increase cmake policy version from 3.12 to 4.1 to stop using old and deprecated CMake policies in standalone builds.
Also stop overriding policy version if a cmake parent project has already set one, so parent projects are able to control which policies are enabled by default based on their own needs. Specifically, in the Bitcoin Core subtree, this change causes the libmultiprocess cmake policy version to increase from 3.12 to 3.22, which is the version Bitcoin Core uses.
This PR also adds a new
newdeps
CI job to test CMake 4.1 and the master branch of Cap'n Proto. This PR doesn't change the minimum version of cmake required to build the project, which is still 3.12.This PR is an alternative to #163 which increases the policy version to 3.31 but doesn't include fixes for CI jobs, and doesn't allow the bitcoin core build to choose a lower policy version. This PR is also an alternative to #175 which sets the policy version to 3.22 like the bitcoin build, but also causes builds with earlier versions of cmake to fail.